Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(inputs.knx_listener): Ignore GroupValueRead requests #15007

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

farmio
Copy link
Contributor

@farmio farmio commented Mar 17, 2024

Summary

Ignore KNX GroupValueRead requests as they don't contain any value. Yet DPT 1 typed requests are treated as telegrams with 0 value.

These are my first lines of Go, so please be patient with me 😬

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #15006

@telegraf-tiger
Copy link
Contributor

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Mar 17, 2024
@farmio
Copy link
Contributor Author

farmio commented Mar 17, 2024

!signed-cla

@userwithoutpassword
Copy link

userwithoutpassword commented Mar 17, 2024

I tested some more DPTs. Yes only DPT1.xxx have this problem

@farmio
Copy link
Contributor Author

farmio commented Mar 17, 2024

Yes, that's because unpacking GroupValueRead data for other DPTs fail at

err := target.datapoint.Unpack(msg.Data)

[inputs.knx_listener] Unpacking data failed: given application data has invalid length

so they have never get to be forwarded anyway.

@userwithoutpassword
Copy link

userwithoutpassword commented Mar 17, 2024

Yes, that's because unpacking GroupValueRead data for other DPTs fail at

err := target.datapoint.Unpack(msg.Data)

[inputs.knx_listener] Unpacking data failed: given application data has invalid length

so they have never get to be forwarded anyway.

Interesting

But anyway there is no need for saving GroupValue_Read Messages because there is no valid data in this messages.

What did u change? Does it now ignore all Read messages?

@farmio
Copy link
Contributor Author

farmio commented Mar 17, 2024

What did u change? Does it now ignore all Read messages?

Yes, as it says in the PR description.

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@farmio thanks for your contribution! Your code looks good but I do have two comments/concerns:

  1. Your current code will spam the log-file as you generate an entry every time a group-read arrives. Please change the logic to only log each address once? Some lines below you can see an example for how to do it...
  2. For some applications it might be advantageous to also log DPT1.xxx events. Could we just log a fixed value if the datagram does not contain any? Like a zero value or similar?

@srebhan srebhan self-assigned this Mar 18, 2024
@srebhan srebhan added the area/iot New plugins or features relating to IoT monitoring label Mar 18, 2024
@farmio
Copy link
Contributor Author

farmio commented Mar 18, 2024

@srebhan Hi 👋!

Ad 1: Depending on the log-level, current implementation will reduce logs, as the new GroupValueRead-log is debug, but a read request for a non-DPT-1-Telegram did produce an error log (see #15007 (comment))
The example you mentioned would filter out (except for the first occurrence) logs for unconfigured GAs (addresses). Is this in your interest?
Or do you want to filter all GAs? I guess we may just remove the log in that case.

Ad 2: DPT 1.x (Boolean value) write and response telegrams are still logged like they have been before. Only the read telegrams are filtered out. I can not think of an application needing to log a read telegram as constant value (without the possibility to distinguish it from writes/responses - which would be possible by eg. adding an additional tag).

@farmio
Copy link
Contributor Author

farmio commented Mar 18, 2024

I moved the match for the message command behind the test for the group address.
Now the GroupValueRead debug log only applies for configured (known) group addresses.
Please let me know if this is what you meant.

@srebhan
Copy link
Member

srebhan commented Mar 18, 2024

@farmio thanks for your explanation! So just to be sure I got it right, your PR filters the group-read-request but not the response!?!? If this is correct, I would not log anything and just go on. Please also add a small comment to the code on why this is required...

If you really want to log a debug message, you should really only log it once for a GA as some people run Telegraf in debug mode in production environments and we would log this every single time we see a group-read request otherwise.

@farmio
Copy link
Contributor Author

farmio commented Mar 18, 2024

You are exactly right. We ignore the read request (that doesn't carry valuable data), but still process the responses (that do carry data). And of course we still process the normal write requests.

Removed the logging and added a comment explaining why GroupValueRead requests are ignored.

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks a lot @farmio for the fix and your explanation!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Mar 18, 2024
@srebhan srebhan assigned powersj and DStrand1 and unassigned srebhan Mar 18, 2024
@DStrand1 DStrand1 removed their assignment Mar 18, 2024
@powersj powersj merged commit a0cf90b into influxdata:master Mar 19, 2024
26 checks passed
@github-actions github-actions bot added this to the v1.30.1 milestone Mar 19, 2024
@farmio farmio deleted the ignore-read branch March 19, 2024 05:16
powersj pushed a commit that referenced this pull request Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/iot New plugins or features relating to IoT monitoring fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inputs.knx_listener is writing wrong "source" tag
5 participants